Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up byline validation #905

Merged
merged 2 commits into from
Sep 26, 2024
Merged

Clean up byline validation #905

merged 2 commits into from
Sep 26, 2024

Conversation

JKingweb
Copy link
Contributor

The _checkByline function is a little weird in a few ways:

  • Despite its name and return type it does more than check
  • It relies on external information to short-curcuit itself
  • It calls another function to do more checks
  • It checks whether the node is an element (which will always be true) before getting attributes, but unconditionally relies on variables it has set conditionally

It gets the job done, but I found it confusing to read and make sense of considering its modest length. I have consequently cleaned it up and re-organized the logic surrounding byline harvesting in general.

I included some basic documentation, but since I've never written a JavaScript doc-string before and I'm uncertain what the matchString argument is actually for, suggestions for improvement there are welcome.

For reference, one can assume that node is an element becaause the _grabArticle loop begins at documentElement, and the _getNextNode function which drives the loop always returns an element if it returns a node.

- Consolidate _checkByline and _isValidByline into one function
- Avoid side effects and short circuiting
- Move pre-existence checks and side effects out to _grabArticle
- Add basic documentation
Copy link
Contributor

@gijsk gijsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks and sorry for the wait!

@gijsk gijsk merged commit bd847da into mozilla:main Sep 26, 2024
1 check passed
@JKingweb JKingweb deleted the byline branch October 11, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants